Skip to content

sdk/basyx/aas/adapter/xml/xml_serialization.py: Remove code duplication #406

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
s-heppner merged 1 commit into from
Aug 19, 2025
Merged

sdk/basyx/aas/adapter/xml/xml_serialization.py: Remove code duplication #406

s-heppner merged 1 commit into from
Aug 19, 2025

Conversation

@moritzsommer
Copy link
Contributor

@moritzsommer moritzsommer commented Aug 11, 2025 •
edited
Loading

Previously, the method object_to_xml_element() in sdk\basyx\aas\adapter\xml\xml_serialization.py contained duplicated code from submodel_element_to_xml() and data_element_to_xml(). This was due to the class hierarchy of the objects being compared in the if statements.

For instance, some models inherit from model.SubmodelElement. Because of that, the associated objects are checked for in submodel_element_to_xml(). With that, any additional check for these objects in object_to_xml_element() are redundant.

This removes the code duplication.

Fixes #396

@moritzsommer moritzsommer changed the title sdk\basyx\aas\adapter\xml\xml_serialization.py: Remove code duplications sdk\basyx\aas\adapter\xml\xml_serialization.py: Remove code duplication Aug 11, 2025
@moritzsommer
sdk\basyx\aas\adapter\xml\xml_serialization.py: Remove code duplication
fecd853 
Previously, the method `object_to_xml_element()` in
`sdk\basyx\aas\adapter\xml\xml_serialization.py` contained duplicated
code from `submodel_element_to_xml()` and `data_element_to_xml()`. This
was due to the class hierarchy of the objects being compared in the if
statements.

For instance, some models inherit from `model.SubmodelElement`. Because
of that, the associated objects are checked for in
`submodel_element_to_xml()`. With that, any additional check for these
objects in `object_to_xml_element()` are redundant.

This removes the code duplication.

Fixes eclipse-basyx#396
@moritzsommer
Copy link
Contributor Author

moritzsommer commented Aug 11, 2025

I am not sure about data_specification_iec61360_to_xml() and data_specification_content_to_xml() tho. On the one hand, data_specification_content_to_xml() only has the job to call data_specification_iec61360_to_xml(), resulting in the additional call of data_specification_iec61360_to_xml() in object_to_xml_element() being redundant. On the other hand, data_specification_content_to_xml() wraps the result of data_specification_iec61360_to_xml() into an additional etree._Element. Technically, this is a different result than calling data_specification_iec61360_to_xml() directly. Is this behaviour intended, or can I just delete data_specification_iec61360_to_xml() from object_to_xml_element() ?

basyx-python-sdk/sdk/basyx/aas/adapter/xml/xml_serialization.py

Lines 936 to 944 in 6f5e4b8

elif isinstance(obj, model.DataSpecificationIEC61360):
return data_specification_iec61360_to_xml(obj)
# generic serialization using the functions for abstract classes
elif isinstance(obj, model.DataElement):
return data_element_to_xml(obj)
elif isinstance(obj, model.SubmodelElement):
return submodel_element_to_xml(obj)
elif isinstance(obj, model.DataSpecificationContent):
return data_specification_content_to_xml(obj)

@moritzsommer moritzsommer changed the title sdk\basyx\aas\adapter\xml\xml_serialization.py: Remove code duplication sdk/basyx/aas/adapter/xml/xml_serialization.py: Remove code duplication Aug 11, 2025
@s-heppner
Copy link
Contributor

s-heppner commented Aug 19, 2025

I am not sure about data_specification_iec61360_to_xml() and data_specification_content_to_xml() tho. On the one hand, data_specification_content_to_xml() only has the job to call data_specification_iec61360_to_xml(), resulting in the additional call of data_specification_iec61360_to_xml() in object_to_xml_element() being redundant. On the other hand, data_specification_content_to_xml() wraps the result of data_specification_iec61360_to_xml() into an additional etree._Element. Technically, this is a different result than calling data_specification_iec61360_to_xml() directly. Is this behaviour intended, or can I just delete data_specification_iec61360_to_xml() from object_to_xml_element() ?

basyx-python-sdk/sdk/basyx/aas/adapter/xml/xml_serialization.py

Lines 936 to 944 in 6f5e4b8

elif isinstance(obj, model.DataSpecificationIEC61360):
return data_specification_iec61360_to_xml(obj)
# generic serialization using the functions for abstract classes
elif isinstance(obj, model.DataElement):
return data_element_to_xml(obj)
elif isinstance(obj, model.SubmodelElement):
return submodel_element_to_xml(obj)
elif isinstance(obj, model.DataSpecificationContent):
return data_specification_content_to_xml(obj)

Yes, I fully agree with you that this is ugly, but sadly it is intended.
The reason for this weird construct is that in theory people can add arbitrary data to the AAS via a DataSpecification. However, obviously no one can actually parse arbitrary data, so in practice there's only one DataSpecification, that is DataSpecificationIEC61360 actually defined in Part 3a of the specification. So in order to be compliant with the specification, we do need this extra wrapped XML element.

Furthermore, our extra function exists in order to future proofs our implementation a little bit (e.g. when Part 3b with a new DataSpecification gets released).

s-heppner
s-heppner approved these changes Aug 19, 2025
View reviewed changes
Copy link
Contributor

@s-heppner s-heppner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@s-heppner s-heppner merged commit 128a2ef into eclipse-basyx:develop Aug 19, 2025
15 checks passed
@s-heppner s-heppner deleted the fix/396 branch August 19, 2025 07:50
@s-heppner s-heppner mentioned this pull request Oct 27, 2025
Merged
zrgt pushed a commit to rwth-iat/basyx-python-sdk that referenced this pull request Nov 18, 2025
@moritzsommer @zrgt
sdk\basyx\aas\adapter\xml\xml_serialization.py: Remove code duplicati…
f281175 
…on (eclipse-basyx#406)

Previously, the method `object_to_xml_element()` in
`sdk\basyx\aas\adapter\xml\xml_serialization.py` contained duplicated
code from `submodel_element_to_xml()` and `data_element_to_xml()`. This
was due to the class hierarchy of the objects being compared in the if
statements.

For instance, some models inherit from `model.SubmodelElement`. Because
of that, the associated objects are checked for in
`submodel_element_to_xml()`. With that, any additional check for these
objects in `object_to_xml_element()` are redundant.

This removes the code duplication.

Fixes eclipse-basyx#396
s-heppner added a commit that referenced this pull request Dec 5, 2025
@s-heppner
Merge pull request #424 from eclipse-basyx/develop
f831016 
Prepare Release v2.0.0

# Release Notes

Version 2.0.0 of the BaSyx-Python SDK comes with a major refactoring of the server and a renewed concept for data persistence.

Previously, the server code was split between the `sdk` and `server` packages (due to historic development of the code). 
Now all the code relevant just for the server is located in `server`, where it belongs. Since this means, some code that was previously in `sdk` is not there anymore, this is a breaking change and warranted the new major release.

> [!note]
> This release does not have any changes in implemented AAS specification versions. It is the preparotory release in order to get ready for the new versions of the specifications, as well as new features for the SDK, such as Registry and Discovery server.

> [!warning]
> Due to these major refactorings, there were some backward incompatible changes. Please check the documentation, if you encounter any issues.

# Changelog

**Notable:**
- Backward Incompatible: Refactor server functionality from `sdk` to `server` (See: #388)
- Backward Incompatible: Refactor `backend` concept for data persistence (See: #370)
- Backward Incompatible: Refactor server `start-up` options (See: #418)
- Remove support for Python 3.9 (as it is EoL) (See #433)

**Improvements:**
- Clarify documentation of running the server with Docker (See: #398)
- Document running the server without Docker (See: #403)
- Improve XML serialization (See: #406)
- Improve server reading of JSON and XML files (See: #408)
- Add more utility methods for `Referable` and `Key` handling (See: #410)

**Bugfixes:**
- Fix type issues found with a new version of `mypy` (See: #399)
- Fix parsing of `ConceptDescription`s in the server (See: #420)
- Update `pyecma376-2` and `lxml` dependencies (See: #419)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@s-heppner s-heppner s-heppner approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@moritzsommer @s-heppner